CPP: add a query for catching alloca in a loop#965
Conversation
|
|
|
Forgot to add a link to the query in action: https://lgtm.com/query/6338234238766417258/ |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Thank you for your contribution @evverx!
Results of this query on 42 projects on LGTM: https://lgtm.com/query/5428973981691801120/ . They appear to be correct from the point of view of what the query is trying to flag, but in some cases because the loops are bounded I think the code is actually safe.
With @precision high these results would be displayed on LGTM by default, which is a high standard for a new query. I'd be happier if we set it to @precision medium or spend longer refining the query.
| where getAnEnclosingLoopOfExpr(fc) = l | ||
| and fc.getTarget().getName() = "__builtin_alloca" | ||
| and not l.(DoStmt).getCondition().getValue() = "0" | ||
| select l, fc |
There was a problem hiding this comment.
There needs to be a proper message in the select. For example:
select fc, "Stack allocation is inside a $@ and could lead to overflow.", l, l.toString()
Thanks to Sam Lanning (@samlanning) and Robert Marsh for taking the time to help to make it possible. In fact, it was Robert Marsh who effectively wrote the query and figured out that __builtin_alloca should be used to also take functions like strdupa into account. I just filled out the metadata :-)
Haha, don't know about perfect ;) I think what I meant was that @rdmarsh2 offered to do the qhelp and unit tests. :) |
|
@geoffw0 thank you for the review! Fair enough, "high" was probably more about what I aspire to :-) I changed the precision to "medium" and updated the select as you suggested. |
|
@rdmarsh2 not at all. I'm not sure at this point I'll be of any help but I think it would be useful for me to take a look at how all the moving parts should be put together to complete the query. And thank you again for your help! |
|
Thank you! How long does it usually take for a query to be rolled out on LGTM? If I understand correctly the results will be hidden by default once the query has reached LGTM. Would it be safe to include it in |
|
There's a continuous testing and deployment (and data migration) cycle of the QL analysis engine and default queries on LGTM.com. Once a query has been merged, it'll become part of that cycle. Yesterday we started running a smoke test for the next deployment: the next version of the analysis engine is being tested on a subset of the ~130,000 LGTM.com projects. Results will be double-checked, and a full test (on all 130k projects) will likely be run early next week, after which the new engine will be made live. Your query is unfortunately not part of the current cycle that started yesterday, which means it'll be part of the next cycle which should result in your query becoming live on LGTM.com in about two weeks. Of course it's possible to run the query as a custom query on LGTM.com in the meantime; more info here: https://lgtm.com/help/lgtm/writing-custom-queries |
|
@evverx It is safe to include the query in the |
6 participants
Thanks to Sam Lanning (@samlanning) and Robert Marsh (@rdmarsh2) for taking the time to help
to make it possible. In fact, it was Robert Marsh who effectively
wrote the query and figured out that __builtin_alloca should be
used to also take functions like strdupa into account. I just
filled out the metadata :-)
@samlanning is my understanding correct that we kind of agreed that the query is perfect and self-documenting so there's no need for me to add tests or documentation? :-)